Codecov Report

Attention: Patch coverage is 38.88889% with 88 lines in your changes are missing coverage. Please review.

Project coverage is 91.80%. Comparing base (920e73b) to head (4189fb0).

@@           Coverage Diff           @@
##             main    #1501   +/-   ##
=======================================
  Coverage   91.80%   91.80%           
=======================================
  Files          40       40           
  Lines       18337    18338    +1     
=======================================
+ Hits        16835    16836    +1     
  Misses       1502     1502           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc before spending effort on reviewing this, I'm interested in you opinion on #1448 (comment). Although the type becomes a bit more complex, it seems overall like a simplification to remove the offset methods from TimeZone and make the enum only generic over Offset types.

I don't love the name?

• Abbreviations on type/field names are kinda unidiomatic
• I don't think the term "resolution" is all that common for resolving a date/time to a timezone offset, at least in non-expert usage?

(I feel like it's probably slightly better than LocalResult but if we're going to do this I'd like to feel better about the name we end up picking.)

Hmm. The result of finding the offset of a local datetime within a timezone, which needs a decision on how to deal with times that fall in a transition. What more names can we think up for it?

ResolveOffset maybe? We need the user to decide what offset he wants in case of a transition.

CurrentOffset, ProjectedOffset, ExpectedOffset?

Those also don't seem to be it. The result is not current, a projection, and there may be no expectation? Resolve, resolution or result seem closer to me.

Maybe something that has 'map' or 'disambiguation' (https://peps.python.org/pep-0495/) in the name? I don't like something as long as disambiguation though.

LocalTimeMapping?

MappedTime?

After looking at the dictionary Mapped is better than Mapping. Then I think we should keep Local in the name, just to make it more clear this is not a type like NaiveDateTime or DateTime. MappedLocalTime?

Seems a reasonable type to return from TimeZone::from_local_datetime. And it also seems explainable from DateTime::add_days or DateTime::with_hour and similar, which do the operation in local time and then have to calculate back an offset.

Seems like a decent option...

The least bad 😆.

With this name I do think it is best to combine it with making the type only return DateTimes and not offsets anymore, the last idea in #1448 (comment). So targeting the 0.5 branch.

Have you already formed an opinion on that?
In short: We remove the TimeZone::offset_from_* methods. An offset can easily be derived from a DateTime with .offset(). That makes the TimeZone trait and this type simpler.

With this name I do think it is best to combine it with making the type only return DateTimes and not offsets anymore, the last idea in #1448 (comment). So targeting the 0.5 branch.

Why/how is that related to the naming? In principle I like the idea of bringing the new name to 0.4.x as an alias to potentially get some feedback.

Have you already formed an opinion on that? In short: We remove the TimeZone::offset_from_* methods. An offset can easily be derived from a DateTime with .offset(). That makes the TimeZone trait and this type simpler.

I have not, and this description doesn't help me form an opinion. Are there commits that flesh out this idea?

Why/how is that related to the naming?

It is a bit strange if TimeZone::offset_from_local_datetime returns an offset in a type with the name MappedLocalTime.
But I'm fine with doing it on 0.4, that method should be little-used.

I have not, and this description doesn't help me form an opinion. Are there commits that flesh out this idea?

I put effort in the description in the linked comment. But I'll try to make some commits after the rename.

Something weird happened with my branches, I'll figure it out